-
Notifications
You must be signed in to change notification settings - Fork 2
Recursion #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for recursive functions to the Amber language analyzer (alpha050 version). The key change is refactoring the function analysis flow to register function symbols in the symbol table before analyzing the function body, which allows functions to reference themselves recursively.
Key Changes:
- Moved function symbol registration to occur before body analysis in
src/analysis/alpha050/global.rs - Added type compatibility rule allowing
Intto matchNumbertype expectations - Added 6 new test files demonstrating various recursion patterns (factorial, fibonacci, ackermann, array sum, mutual recursion, post-order)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/analysis/alpha050/global.rs |
Refactored to insert function symbol into symbol table before analyzing function body, enabling recursive calls |
src/analysis/types.rs |
Added type matching rule for Int as subtype of Number |
tests/analysis/alpha050.rs |
Added test case for recursion with Ackermann function |
tests/analysis/snapshots/r#mod__analysis__alpha050__recursion_ackermann.snap.new |
Snapshot of expected symbol table output for recursion test |
tests/analysis/recursion_ackermann.ab |
Test file for Ackermann function (doubly-recursive) |
tests/analysis/recursion_factorial.ab |
Test file for factorial function (simple recursion) |
tests/analysis/recursion_fibonacci.ab |
Test file for Fibonacci function (double recursion) |
tests/analysis/recursion_array.ab |
Test file for recursive array sum |
tests/analysis/recursion_mutual.ab |
Test file for mutually recursive functions |
tests/analysis/recursion_post_order.ab |
Test file for post-order recursion with side effects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| contexts: vec![], | ||
| }, | ||
| (file_id, file_version), | ||
| 0..=usize::MAX, |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 0..=usize::MAX as the definition scope allows the function to be referenced from anywhere in the file, including before its definition. This enables recursion but also permits forward references that may be unintended.
Consider using name_span.start..=usize::MAX instead, which would make the function visible from the point of its declaration forward (including within its own body for recursion) while preventing forward references from earlier in the file. This would be more consistent with typical scoping rules.
| 0..=usize::MAX, | |
| name_span.start..=usize::MAX, |
| arguments: args | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(idx, (arg, span))| match arg { | ||
| FunctionArgument::Generic((is_ref, _), (name, _)) => { | ||
| Some(( | ||
| analysis::FunctionArgument { | ||
| name: name.clone(), | ||
| data_type: DataType::Generic( | ||
| new_generic_types[idx], | ||
| ), | ||
| is_optional: false, | ||
| is_ref: *is_ref, | ||
| }, | ||
| *span, | ||
| )) | ||
| } | ||
| FunctionArgument::Typed( | ||
| (is_ref, _), | ||
| (name, _), | ||
| (ty, _), | ||
| ) => Some(( | ||
| analysis::FunctionArgument { | ||
| name: name.clone(), | ||
| data_type: ty.clone(), | ||
| is_optional: false, | ||
| is_ref: *is_ref, | ||
| }, | ||
| *span, | ||
| )), | ||
| FunctionArgument::Optional( | ||
| (is_ref, _), | ||
| (name, _), | ||
| ty, | ||
| _, | ||
| ) => Some(( | ||
| analysis::FunctionArgument { | ||
| name: name.clone(), | ||
| data_type: match ty { | ||
| Some((ty, _)) => ty.clone(), | ||
| None => { | ||
| DataType::Generic(new_generic_types[idx]) | ||
| } | ||
| }, | ||
| is_optional: true, | ||
| is_ref: *is_ref, | ||
| }, | ||
| *span, | ||
| )), | ||
| FunctionArgument::Error => None, | ||
| }) | ||
| .collect::<Vec<_>>(), |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential index out of bounds error when accessing new_generic_types[idx]. The variable idx represents the position in the args array, but new_generic_types only contains entries for generic and optional arguments without explicit types.
For example, with fun foo(a: Num, b), args has 2 elements but new_generic_types has only 1. When processing argument at index 0 (a: Num), the code would try to access new_generic_types[0] in the Typed branch (line 236), which doesn't make sense since typed arguments don't add to new_generic_types. When processing argument at index 1 (b), it would try new_generic_types[1] which is out of bounds.
The old code used new_generic_types.remove(0) which correctly dequeued generics in order. Consider maintaining a separate counter for generic indices or reverting to the dequeue approach.
| arguments: args | |
| .iter() | |
| .enumerate() | |
| .filter_map(|(idx, (arg, span))| match arg { | |
| FunctionArgument::Generic((is_ref, _), (name, _)) => { | |
| Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: DataType::Generic( | |
| new_generic_types[idx], | |
| ), | |
| is_optional: false, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )) | |
| } | |
| FunctionArgument::Typed( | |
| (is_ref, _), | |
| (name, _), | |
| (ty, _), | |
| ) => Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: ty.clone(), | |
| is_optional: false, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )), | |
| FunctionArgument::Optional( | |
| (is_ref, _), | |
| (name, _), | |
| ty, | |
| _, | |
| ) => Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: match ty { | |
| Some((ty, _)) => ty.clone(), | |
| None => { | |
| DataType::Generic(new_generic_types[idx]) | |
| } | |
| }, | |
| is_optional: true, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )), | |
| FunctionArgument::Error => None, | |
| }) | |
| .collect::<Vec<_>>(), | |
| arguments: { | |
| let mut new_generic_types = new_generic_types.clone(); | |
| args | |
| .iter() | |
| .enumerate() | |
| .filter_map(|(_idx, (arg, span))| match arg { | |
| FunctionArgument::Generic((is_ref, _), (name, _)) => { | |
| Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: DataType::Generic( | |
| new_generic_types.remove(0), | |
| ), | |
| is_optional: false, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )) | |
| } | |
| FunctionArgument::Typed( | |
| (is_ref, _), | |
| (name, _), | |
| (ty, _), | |
| ) => Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: ty.clone(), | |
| is_optional: false, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )), | |
| FunctionArgument::Optional( | |
| (is_ref, _), | |
| (name, _), | |
| ty, | |
| _, | |
| ) => Some(( | |
| analysis::FunctionArgument { | |
| name: name.clone(), | |
| data_type: match ty { | |
| Some((ty, _)) => ty.clone(), | |
| None => { | |
| DataType::Generic(new_generic_types.remove(0)) | |
| } | |
| }, | |
| is_optional: true, | |
| is_ref: *is_ref, | |
| }, | |
| *span, | |
| )), | |
| FunctionArgument::Error => None, | |
| }) | |
| .collect::<Vec<_>>() | |
| }, |
| let symbol_table = backend.files.symbol_table.get(&file_id).unwrap(); | ||
| let generic_types = backend.files.generic_types.clone(); | ||
|
|
||
| // When both operands are unknown generics, they should get the broad constraint |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "When both operands are unknown generics, they should get the broad constraint" appears to be copy-pasted from another test and is not relevant to this recursion test. The ackermann function uses typed parameters (m: Num, n: Num) and doesn't involve generic constraints. Consider updating the comment to describe what this test is actually verifying (e.g., "Test that recursive function calls are properly analyzed and symbol table is correctly populated").
| // When both operands are unknown generics, they should get the broad constraint | |
| // Test that recursive function calls are properly analyzed and symbol table is correctly populated |
No description provided.